Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL] Refactor sycl::vec's operators implementation #16529

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

aelovikov-intel
Copy link
Contributor

  • Don't use sycl::vec::vector_t, as it is planned to be removed from the SYCL 2020 (Remove underspecified vector_t KhronosGroup/SYCL-Docs#676). Note that this implementation is NOT required to use it, so this PR can be merged before the specification change.
  • Use ext_vector_type-based optimized implementation whenever it's available and not on device only.

* Don't use `sycl::vec::vector_t`, as it is planned to be removed from
  the SYCL 2020 (KhronosGroup/SYCL-Docs#676).
  Note that this implementation is NOT required to use it, so this PR
  can be merged before the specification change.
* Use `ext_vector_type`-based optimized implementation whenever it's
  available and not on device only.
@aelovikov-intel aelovikov-intel marked this pull request as ready for review January 6, 2025 21:11
@aelovikov-intel aelovikov-intel requested a review from a team as a code owner January 6, 2025 21:11
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Do you think it would be worth it to also add a test to check the LLVM IR generated on host, with and without this optimization? Something similar to check_device_code/vector/vector_math_ops.cpp?

@aelovikov-intel
Copy link
Contributor Author

LGTM. Do you think it would be worth it to also add a test to check the LLVM IR generated on host, with and without this optimization? Something similar to check_device_code/vector/vector_math_ops.cpp?

I don't see any benefits in that. It's either exactly the same as for device, or old naive implementation, depending on the host compiler.

@aelovikov-intel aelovikov-intel marked this pull request as draft January 6, 2025 23:29
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM!

sycl/include/sycl/detail/vector_arith.hpp Show resolved Hide resolved
sycl/include/sycl/detail/vector_arith.hpp Show resolved Hide resolved
@steffenlarsen steffenlarsen merged commit 16c2c21 into intel:sycl Jan 8, 2025
17 checks passed
steffenlarsen added a commit that referenced this pull request Jan 8, 2025
steffenlarsen added a commit that referenced this pull request Jan 8, 2025
Reverts #16529 due to failure to build on Windows and MacOS in
post-commit.
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to revert this patch due to failures in post-commit builds on Windows and MacOS.

// `std::array<bool, N>` is different and LLVM annotates its
// elements with [0, 2) range metadata when loaded, so we need to
// ensure we generate 0/1 only (and not 2/-1/etc.).
static_assert((ext_vector<int8_t, 2>{1, 0} == 0)[1] == -1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to be a constant expression on Windows and MacOS.

@aelovikov-intel aelovikov-intel deleted the refactor-vec-ops branch January 8, 2025 15:07
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jan 8, 2025
* Don't use `sycl::vec::vector_t`, as it is planned to be removed from
the SYCL 2020 (KhronosGroup/SYCL-Docs#676). Note
that this implementation is NOT required to use it, so this PR can be
merged before the specification change.
* Use `ext_vector_type`-based optimized implementation whenever it's
available and not on device only.

This is a recommit of intel#16529 with an
additional `#if __clang_major__ >= 20` guard around `static_assert`
on the expression that wasn't constant in clang-19.
aelovikov-intel added a commit that referenced this pull request Jan 8, 2025
* Don't use `sycl::vec::vector_t`, as it is planned to be removed from
the SYCL 2020 (KhronosGroup/SYCL-Docs#676). Note
that this implementation is NOT required to use it, so this PR can be
merged before the specification change.
* Use `ext_vector_type`-based optimized implementation whenever it's
available and not on device only.

This is a recommit of #16529 with an
additional `#if __clang_major__ >= 20` guard around `static_assert` on
the expression that wasn't constant in clang-19.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants